Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add openmetrics format parser #669

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jyz0309
Copy link

@jyz0309 jyz0309 commented Jul 22, 2024

Ref: prometheus/issue/8932
Add openmetrics protocol decoder, for promtool to support check openmetric protocol

Signed-off-by: jyz0309 <[email protected]>
Signed-off-by: jyz0309 <[email protected]>
Signed-off-by: jyz0309 <[email protected]>
Signed-off-by: jyz0309 <[email protected]>
Signed-off-by: jyz0309 <[email protected]>
Signed-off-by: jyz0309 <[email protected]>
@jyz0309
Copy link
Author

jyz0309 commented Jul 22, 2024

would love to get your review/opinion about this :-) @beorn7

@beorn7 beorn7 self-requested a review July 23, 2024 13:23
@beorn7
Copy link
Member

beorn7 commented Jul 23, 2024

This in my review queue (but the queue is long, so if somebody beats me to it, even better).

@beorn7
Copy link
Member

beorn7 commented Jul 25, 2024

I have reached the beginning of my (unplugged) vacations and didn't get to this. I'm very sorry for that. I'll try to find another reviewer (or will come back to this in about two weeks time).

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋, that's quite a big PR 😬 Thank you for the effort here, I'm sure it took a long time to write tests with so much coverage.

I'm also traveling today and won't have internet access until the beginning of August.

I'm doing a quick review here just so you have something to work on while the reviewers are away, I can do a longer review once I'm back

expfmt/decode.go Outdated Show resolved Hide resolved
expfmt/decode.go Outdated Show resolved Hide resolved
expfmt/decode_test.go Outdated Show resolved Hide resolved
expfmt/openmetrics_parse_test.go Outdated Show resolved Hide resolved
expfmt/openmetrics_parse_test.go Outdated Show resolved Hide resolved
expfmt/openmetrics_parse.go Outdated Show resolved Hide resolved
@jyz0309
Copy link
Author

jyz0309 commented Aug 1, 2024

I have updated the code according to the code review comments. PTAL If you have time @ArthurSens

@beorn7
Copy link
Member

beorn7 commented Aug 8, 2024

@ArthurSens I assume you'll take care of the further review. Let me know if you need any help.

@ArthurSens
Copy link
Member

Yes, I plan to continue the review! But if you find time to also give a review, please do :)

Onboarding at the new job has been very time-consuming. Too many videos to watch, and too many trainings to attend 😅

@jyz0309 jyz0309 requested a review from ArthurSens August 9, 2024 16:40
@beorn7
Copy link
Member

beorn7 commented Aug 14, 2024

We are clearly both lacking time. Which in turn means we should try even harder to avoid redundant review work. Instead of a parallel review, one of us should do the main review, and only loop in the other where needed (open questions of any kind, tough calls to make, …).

@beorn7
Copy link
Member

beorn7 commented Sep 5, 2024

@ArthurSens is this still on your queue?

@ArthurSens
Copy link
Member

@ArthurSens is this still on your queue?

Yes, it is! To be completely honest with you, @jyz0309, I've opened this PR to review quite a few times in the past weeks, but I've been feeling a bit overwhelmed with its size, so I ended up procrastinating the review.

I wonder if we could split this PR into smaller ones, to make the review process smoother and we're able to advance here. What do you think of the idea? Would that be ok?

If we split, we can start with a super simple parser implementing each OpenMetrics feature in parts. For example:

  • First PR: a parser that can parse HELP, TYPE, and UNIT, plus tests that assert this works.
  • Second PR: extend the parser to also parse counters and gauges, not including fancy features like timestamps, exemplars, etc. Tests are also extended and previous tests need to keep passing.
  • Third PR: Let's parse summaries/histograms

etc etc, until we cover all OM features.

What do you think? 🙂

@jyz0309
Copy link
Author

jyz0309 commented Sep 7, 2024

@ArthurSens is this still on your queue?

Yes, it is! To be completely honest with you, @jyz0309, I've opened this PR to review quite a few times in the past weeks, but I've been feeling a bit overwhelmed with its size, so I ended up procrastinating the review.

I wonder if we could split this PR into smaller ones, to make the review process smoother and we're able to advance here. What do you think of the idea? Would that be ok?

If we split, we can start with a super simple parser implementing each OpenMetrics feature in parts. For example:

  • First PR: a parser that can parse HELP, TYPE, and UNIT, plus tests that assert this works.
  • Second PR: extend the parser to also parse counters and gauges, not including fancy features like timestamps, exemplars, etc. Tests are also extended and previous tests need to keep passing.
  • Third PR: Let's parse summaries/histograms

etc etc, until we cover all OM features.

What do you think? 🙂

Okay, I will finish this work next week(maybe by next Friday)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants